-
Notifications
You must be signed in to change notification settings - Fork 654
fix(expect,testing,internal): throw if expect.hasAssertion
and expect.assertions
are not checked
#6646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6646 +/- ##
==========================================
+ Coverage 94.75% 94.81% +0.06%
==========================================
Files 584 574 -10
Lines 46563 46439 -124
Branches 6541 6528 -13
==========================================
- Hits 44120 44032 -88
+ Misses 2400 2364 -36
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thank you for the PR! This looks like a nice fix, but I left some comments about code organization. |
@kt3k I've rewritten this PR to modify as little of the behavior as possible. Note that |
Thanks for working on this! |
const { stderr } = await command.output(); | ||
const errorMessage = stripAnsiCode(new TextDecoder().decode(stderr)); | ||
// TODO(WWRS): Test for the expected message when Deno displays it instead of "Uncaught null" | ||
assertStringIncludes(errorMessage, "error: Uncaught"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be the message generated in unload
handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is an upstream issue, which I've opened: denoland/deno#29087
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating! This now looks good to me 👍
Nice fixes!
expect.hasAssertion
and expect.assertions
are not checkedexpect.hasAssertion
and expect.assertions
are not checked
fixes #6518
fixes #6540
Renamed AssertionState to AssertionCounterMoved AssertionCounter fromstd/internal
tostd/expect
: It's really more of anexpect
feature than aninternal
featureThis means thatstd/testing
now depends onstd/expect
RemovedAssertionCounter.#state.assertionTriggered
to avoid desynchronizing fromassertionTriggeredCount
Collected the validation error behavior inAssertionCounter
rather than duplicating it intesting
AssertionState
is reset after validation: fixesexpect.hasAssertion
andexpect.assertions
pollute other assertions tests #6540globalThis
isunload
ed to ensure allAssertionState
s are properly validated and reset: fixesexpect.hasAssertion
andexpect.assertions
silently pass outside bdd tests #6518